-
-
Notifications
You must be signed in to change notification settings - Fork 15k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci/eval: make eval for non-native platforms less incorrect #378922
ci/eval: make eval for non-native platforms less incorrect #378922
Conversation
We commonly use platform-dependent conditional patterns like `lib.meta.availableOn stdenv.hostPlatform` and `stdenv.hostPlatform.isLinux` to enable different features in a given derivation or to evaluate completely different derivations based on the platform. For example, source builds of a given derivation may only be available on linux but not on darwin. The use of such conditionals allow us to fall back to patched binaries on darwin instead. In `chromedriver` (pkgs/development/tools/selenium/chromedriver/default.nix), we use ~~~nix if lib.meta.availableOn stdenv.hostPlatform chromium then callPackage ./source.nix { } else callPackage ./binary.nix { } ~~~ To provide some context, `chromedriver` source builds are based on `chromium.mkDerivation` and `chromium` is limited to `lib.platforms.linux`. Based on the same `chromium.mkDerivation`, we also do source builds for `electron` (pkgs/top-level/all-packages.nix): ~~~nix electron_33 = if lib.meta.availableOn stdenv.hostPlatform electron-source.electron_33 then electron-source.electron_33 else electron_33-bin; electron_34 = electron_34-bin; electron = electron_34; ~~~ And finally, the top-level `jdk` (Java) attribute has a lot of indirection, but eventually also boils down to `stdenv.hostPlatform.isLinux` for source builds and binaries for x86_64-darwin and aarch64-darwin. A surprising amount of electron and jdk consumers use variations of `meta.platforms = electron.meta.platforms` in their own meta block. Due to internal implementation details, the conditionals in those top-level attributes like `chromedriver`, `electron` and `jdk` are evaluated based on the value from `builtins.currentSystem` and not the system passed to `import <nixpkgs> { }`. This then causes `chromedriver`, `electron`, `jdk` and all dependents that inherit those `meta.platforms` to appear only available on linux despite also being available on darwin. Hydra is affected similarly, but it's a lot more nuanced and in practice not actually *that* bad. The addition of `--eval-system` ensures that `builtins.currentSystem` matches the requested platform. As a bonus, this also fixes the store paths of an impure test that should probably be made pure: ~~~diff @@ -885069,13 +886119,13 @@ "out": "/nix/store/lb2500hc69czy4sfga9mbh2k679cr1rp-test-compressDrv" }, "tests.config.allowPkgsInPermittedInsecurePackages.aarch64-darwin": { - "out": "/nix/store/0l5h8svrpzwymq35mnpvx82gyc7nf8s4-hello-2.12.1" + "out": "/nix/store/v1zjb688mp4y2132b6chii43d5kkxnpa-hello-2.12.1" }, "tests.config.allowPkgsInPermittedInsecurePackages.aarch64-linux": { - "out": "/nix/store/0l5h8svrpzwymq35mnpvx82gyc7nf8s4-hello-2.12.1" + "out": "/nix/store/hb21z2zdk03dwygsw5lvpa8zc3fbr500-hello-2.12.1" }, "tests.config.allowPkgsInPermittedInsecurePackages.x86_64-darwin": { - "out": "/nix/store/0l5h8svrpzwymq35mnpvx82gyc7nf8s4-hello-2.12.1" + "out": "/nix/store/gljdqsf0mxv1j8zb04phx9ws09pp7z3l-hello-2.12.1" }, "tests.config.allowPkgsInPermittedInsecurePackages.x86_64-linux": { "out": "/nix/store/0l5h8svrpzwymq35mnpvx82gyc7nf8s4-hello-2.12.1" ~~~ Diff stats between two full evals based on 75c8548 with and without this fix on x86_64-linux: ~~~bash # git diff --no-index --stat /nix/store/659l3xp78255wx7abbahggsnrlj3a1la-combined-result/outpaths.json /nix/store/4fhlq4g5qa65cxbibskq9pma40zigrx7-combined-result/outpaths.json /nix/store/{659l3xp78255wx7abbahggsnrlj3a1la-combined-result => 4fhlq4g5qa65cxbibskq9pma40zigrx7-combined-result}/outpaths.json | 1416 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 1405 insertions(+), 11 deletions(-) ~~~ The full diff is available as a gist at <https://gist.github.com/emilylange/d40c50031fc332bbcca133ad56d224f6>. When we added `electron_34` only as binary instead of the usual source on linux with binary fallback in cfed9a1 and made the unversioned `electron` top-level point to the newly added `electron_34` instead of `electron_33`, the GitHub workflow suddenly reported 20 new packages. Of those 20 reported packages, 17 where false-positives caused by dropping the wrongly evaluated conditional.
4f54068
to
657c689
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
Could you point me to where that happens? I can't find it. |
Hm sure, I can try. If you want me to go into more detail, let me know. Lines 91 to 102 in 05397ad
calls nixpkgs/pkgs/top-level/release-attrpaths-parallel.nix Lines 19 to 22 in 66aea4c
which in turn calls nixpkgs/pkgs/top-level/release-outpaths.nix Lines 21 to 25 in 05397ad
nixpkgs/pkgs/top-level/release.nix Lines 12 to 15 in 66aea4c
and there Even if multiple nixpkgs/pkgs/top-level/release.nix Lines 61 to 65 in 66aea4c
nixpkgs/pkgs/top-level/release-lib.nix Line 40 in 66aea4c
Changing We could also modify the chain of imports to pass But then that doesn't fix the impure test I mentioned in my commit and opening comment. |
Ah, thanks. So the This whole thing seems to be equivalent to "Should we run eval for the different platforms on those platforms directly?". Like, right now we run all of the 4 eval jobs on x86_64-linux. We could run them on the other 3 available GitHub runners, though - aarch64-linux, and both as -darwin as well. Would we expect eval to be any worse in that case? Surely not. Clearly, we could only benefit from having If we can achieve the same with this setting and we don't need to switch to other runners (which not be available in the same number as the x86_64 runners) - even better. So I don't see any reason why we should not to this. |
I made the same change locally and ran eval for all 4 systems. Nothing broken. |
Successfully created backport PR for |
We commonly use platform-dependent conditional patterns like
lib.meta.availableOn stdenv.hostPlatform
andstdenv.hostPlatform.isLinux
to enable different features in a given derivation or to evaluate completely different derivations based on the platform.For example, source builds of a given derivation may only be available on linux but not on darwin. The use of such conditionals allow us to fall back to patched binaries on darwin instead.
In
chromedriver
(pkgs/development/tools/selenium/chromedriver/default.nix), we useTo provide some context,
chromedriver
source builds are based onchromium.mkDerivation
andchromium
is limited tolib.platforms.linux
. Based on the samechromium.mkDerivation
, we also do source builds forelectron
(pkgs/top-level/all-packages.nix):And finally, the top-level
jdk
(Java) attribute has a lot of indirection, but eventually also boils down tostdenv.hostPlatform.isLinux
for source builds and binaries for x86_64-darwin and aarch64-darwin.A surprising amount of electron and jdk consumers use variations of
meta.platforms = electron.meta.platforms
in their own meta block. Due to internal implementation details, the conditionals in those top-level attributes likechromedriver
,electron
andjdk
are evaluated based on the value frombuiltins.currentSystem
and not the system passed toimport <nixpkgs> { }
.This then causes
chromedriver
,electron
,jdk
and all dependents that inherit thosemeta.platforms
to appear only available on linux despite also being available on darwin. Hydra is affected similarly, but it's a lot more nuanced and in practice not actually that bad.The addition of
--eval-system
ensures thatbuiltins.currentSystem
matches the requested platform.As a bonus, this also fixes the store paths of an impure test that should probably be made pure:
Diff stats between two full evals based on 75c8548 with and without this fix on x86_64-linux:
The full diff is available as a gist at https://gist.github.com/emilylange/d40c50031fc332bbcca133ad56d224f6.
When we added
electron_34
only as binary instead of the usual source on linux with binary fallback in #376770 (cfed9a1) and made the unversionedelectron
top-level point to the newly addedelectron_34
instead ofelectron_33
, the GitHub workflow suddenly reported 20 new packages. Of those 20 reported packages, 17 where false-positives caused by dropping the wrongly evaluated conditional.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)